feat(integrations): add oauth and hidden field types#4639
Conversation
miguelpeixe
force-pushed
the
feat/integration-oauth-field-type
branch
from
f00a8a9 to
33bb748
Compare
There was a problem hiding this comment.
Pull request overview
Adds two new Reader Activation integration settings field types (oauth and hidden) to the Audience → Integrations UI, plus a defensive guard in the reader-activation client store to prevent merge-strategy errors from throwing during rehydration.
Changes:
- Add
oauthandhiddenfield type handling in the integrations settings UI (connect/disconnect buttons, and non-rendered hidden fields). - Add a try/catch wrapper around
rehydrateItem()merge logic and log a warning on failure. - Allow
oauth/hiddensettings values to bypass sanitization inIntegration::sanitize_settings_field_value().
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| src/wizards/audience/views/integrations/settings-field.js | Adds UI rendering for oauth and hidden integration settings field types. |
| src/reader-activation/store.js | Wraps rehydration merge strategy execution in try/catch and logs on errors. |
| includes/reader-activation/integrations/class-integration.php | Adds hidden/oauth cases to settings-value sanitization behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review feedback on the new oauth settings field: - Surface help_url alongside description by reusing the shared help fragment, matching other field types. - Avoid passing an empty href to the Connect button when no oauth_url is configured, so the disabled button no longer navigates anywhere if clicked.
…e throws
Address review feedback on the rehydrateItem error handler:
- Fix the warning message typo ("rehydrated" -> "rehydrate").
- Overwrite the key with the server value when a merge strategy throws,
matching the no-merge branch so a single bad merge strategy can't
leave the store with the key untouched.
- Add a test that registers a throwing merge strategy and asserts the
store falls back to the server value without throwing.
Address review feedback on sanitize_settings_field_value(): - Reject non-scalar payloads for the new oauth and hidden field types (return field default or empty string) so REST settings can't store arrays/objects where a scalar string is expected. - Sanitize scalar strings through sanitize_text_field, matching how text and password fields are handled. - Add PHPUnit coverage for the new types via a public test wrapper on Sample_Integration, covering scalar sanitization and non-scalar rejection with and without an explicit default.
miguelpeixe
added
the
[Status] Needs Review
adekbadek
left a comment
adekbadek
left a comment
There was a problem hiding this comment.
Verified working in-browser. Suggestions inline.
| $type = $field['type'] ?? 'text'; | ||
| switch ( $type ) { | ||
| case 'hidden': | ||
| case 'oauth': |
There was a problem hiding this comment.
Suggestion: oauth/hidden values are documented as managed but stay client-writable — The comment frames these as "read-only or managed programmatically," but the sanitizer is reached from the admin settings-save REST path, so an admin client can still POST an arbitrary scalar and overwrite a stored value here. It's capability-gated, so this is a data-integrity nuance rather than a security issue, and there's no live consumer yet. Still, if oauth credentials are meant to be set only by a server-side OAuth callback, consider treating these types as read-only on the write path (ignore inbound writes / return the existing stored value) rather than persisting whatever scalar arrives.
There was a problem hiding this comment.
Fixed in f12fe78 — relocated the read-only check from the sanitizer to the REST settings save entry point (Integrations::update_integration_settings()). It now drops keys whose field type is in Integration::MANAGED_FIELD_TYPES (oauth, hidden), so an admin POSTing those keys can no longer overwrite stored values. Server-side writers calling Integration::update_settings_field_value() directly are unaffected.
This matters because newspack-manager#565 — the Salesforce NPC integration — is the first live consumer of these field types. It writes access_token, refresh_token, instance_url from a server-side OAuth callback / token-refresh handler / disconnect handler, all via update_settings_field_value. Putting the check inside the sanitizer would have blocked those legitimate writes; gating the REST entry point preserves them while still defending against admin-client overwrites. Added a test covering both paths.
| if ( ! is_scalar( $value ) ) { | ||
| return $field['default'] ?? ''; | ||
| } | ||
| return \sanitize_text_field( (string) $value ); |
There was a problem hiding this comment.
Suggestion: sanitize_text_field assumes single-line, tag-free values — sanitize_text_field() strips tags and newlines and collapses whitespace, which is correct for the single-line OAuth tokens/JWTs this PR targets. But the case is shared with the generic hidden bucket — if a future hidden field stores a multiline secret (a PEM key, a serialized blob, a refresh token containing <), the value would be silently corrupted with no error. Worth a one-line note on the case that the assumption is "single-line, tag-free scalar" so the next person adding a hidden field doesn't lose data quietly.
There was a problem hiding this comment.
Addressed in f12fe78 — added an inline note on the sanitize_text_field line documenting the "single-line, tag-free scalar" assumption, so the next person adding a hidden field with a multiline payload (PEM, refresh token containing <, etc.) won't lose data silently.
Note: the same commit also moves the admin-write block out to the REST entry point (see the adjacent thread), so sanitize_text_field is no longer reached for these types from the settings REST endpoint at all. The note still applies to any server-side writer that funnels through update_settings_field_value.
github-actions
Bot
added
[Status] Approved
dkoo
left a comment
dkoo
left a comment
There was a problem hiding this comment.
Not doing a full code review, just approving to acknowledge the update_option change in 32ac489 as suggested in https://github.com/Automattic/newspack-manager/pull/565
|
Hey @miguelpeixe, good job getting this PR merged! 🎉 Now, the Please check if this PR needs to be included in the "Upcoming Changes" and "Release Notes" doc. If it doesn't, simply remove the label. If it does, please add an entry to our shared document, with screenshots and testing instructions if applicable, then remove the label. Thank you! ❤️ |
All Submissions:
Changes proposed in this Pull Request:
Adds two new settings field types for reader activation integrations so integrations can expose OAuth-based connections and programmatically-managed values in the Audience integrations UI.
oauthfield type: renders a Connect/Disconnect UI insettings-field.js. When no value is stored it shows a primary Connect button linking tofield.oauth_url(disabled when no URL is provided). When connected it shows the stored value plus a destructive Disconnect button linking tofield.disconnect_url(only rendered when that URL is present).hiddenfield type: renders nothing in the UI, for values that are managed programmatically rather than edited by the user.class-integration.php): bothoauthandhiddenvalues pass throughsanitize_settings_field_value()unchanged, since they are read-only or managed programmatically.Also includes a small defensive fix wrapping
rehydrateItem()in the reader-activation store in a try/catch so a failing merge strategy logs a warning instead of throwing.How to test the changes in this Pull Request:
type => 'oauth'and anoauth_url.disconnect_urlis set).type => 'hidden'and confirm it renders nothing while its value is still persisted on save.oauthandhiddenvalues are returned unchanged throughsanitize_settings_field_value()on save.Other information: